fix shared on windows #461

SpaceIm · 2020-04-07T14:00:28Z

closes #458
closes #516

This big patch comes with several modifications:

FCL_EXPORT is renamed FCL_API
FCL_EXPORT (aka FCL_API now) is removed in all template declarations.
2 new definitions are introduced to handle explicit template declaration and explicit template definition for all compilers: FCL_EXTERN_TEMPLATE_API and FCL_INSTANTIATION_DEF_API. They are needed because Visual Studio expect dllexport in explicit template definition, while others compilers expect this kind of attribute in explicit template declaration.
These macros are defined through an option in generate_export_header (CUSTOM_CONTENT_FROM_VARIABLE) introduced in CMake 3.7. Therefore minimum version of CMake is bumped to 3.7 (maybe undesirable).
It also adds more exported symbols.
~~(my IDE has also removed a lot of trailing spaces).~~ => not anymore

This change is

SeanCurtis-TRI

Reviewed 2 of 357 files at r1.
Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @SpaceIm)

CMakeLists.txt, line 34 at r1 (raw file):

# POSSIBILITY OF SUCH DAMAGE.

cmake_minimum_required(VERSION 3.7)

nit Unilateral changes to the build system are a bit disconcerting.

@jamiesnape can you weigh in on this change?

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

configure_file(config.h.in config.h @ONLY)

set(FCL_EXPORT_MACRO_NAME "FCL_API")

@jamiesnape could you look at this as well.

(NIT: the presence of a bunch of comments is never a good thing.)

SpaceIm

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape)

CMakeLists.txt, line 34 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit Unilateral changes to the build system are a bit disconcerting.

@jamiesnape can you weigh in on this change?

As I said, it is required if we want to inject custom string in export.h
Otherwise it would require more refactoring in order to avoid breaking include logic.

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

@jamiesnape could you look at this as well.

(NIT: the presence of a bunch of comments is never a good thing.)

which comments? Are you talking about /* We are building this library */ ? It's just to be consistent with content of export.h, it can be removed.

SeanCurtis-TRI

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape)

CMakeLists.txt, line 34 at r1 (raw file):

Previously, SpaceIm wrote…

As I said, it is required if we want to inject custom string in export.h
Otherwise it would require more refactoring in order to avoid breaking include logic.

I'm being cautious. Making it work on windows has to be compatible with the other domains that we support. I'm largely CMake-ignorant, so I can't review this change in that context; hence calling in an expert.

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, SpaceIm wrote…

which comments? Are you talking about /* We are building this library */ ? It's just to be consistent with content of export.h, it can be removed.

Ah....I think I see my confusion:

My aforementioned CMake incompetency
My superficial glance at the change led me to think lines like: # define FCL_EXTERN_TEMPLATE_API was a CMake comment.
The separation between # and define contributes to that. Why isn't it #define?

jamiesnape

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape, @SeanCurtis-TRI, and @SpaceIm)

CMakeLists.txt, line 34 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'm being cautious. Making it work on windows has to be compatible with the other domains that we support. I'm largely CMake-ignorant, so I can't review this change in that context; hence calling in an expert.

If we want to continue to support Trusty, we need minimum 2.8.12, if we want to continue to support Xenial, we need minimum 3.5.

I am hoping to retool the build system for when we drop support for those platforms, but we are not at that stage yet, and I do not want to incrementally bump the CMake minimum version from 3.7 to 3.10 as we add features, since it adds complexity to our testing infrastructure (3.6-3.9 are not in an Ubuntu LTS).

Therefore, I am afraid, we cannot bump the version to 3.7.

jamiesnape

Note if we are going to remove large numbers trailing spaces, it should be a separate PR that does nothing else.

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape, @SeanCurtis-TRI, and @SpaceIm)

jamiesnape

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @SeanCurtis-TRI and @SpaceIm)

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ah....I think I see my confusion:

My aforementioned CMake incompetency

My superficial glance at the change led me to think lines like: # define FCL_EXTERN_TEMPLATE_API was a CMake comment.

The separation between # and define contributes to that. Why isn't it #define?

FYI Some preprocessors need the # to be in the first column of the line, so indenting preprocessor definitions is usually done after the #.

SpaceIm · 2020-04-09T18:40:12Z

CMakeLists.txt, line 34 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

If we want to continue to support Trusty, we need minimum 2.8.12, if we want to continue to support Xenial, we need minimum 3.5.

I am hoping to retool the build system for when we drop support for those platforms, but we are not at that stage yet, and I do not want to incrementally bump the CMake minimum version from 3.7 to 3.10 as we add features, since it adds complexity to our testing infrastructure (3.6-3.9 are not in an Ubuntu LTS).

Therefore, I am afraid, we cannot bump the version to 3.7.

The only reason of 3.7 is the option CUSTOM_CONTENT_FROM_VARIABLE I use in generate_export_header in order to inject explicit template instantiation and definition definitions. There should be another way to achieve the same behaviour without this option.
For example, a less elegant solution could be to rename export.h, and create a file export.h with macros related to template instantiation which includes the file generated by generate_export_header. Thereby, we can rely of the current include behaviour of export.h in other files.

SpaceIm · 2020-04-09T18:42:33Z

Honestly, I can't imagine redo all these modifications (357 files!) just to remove modifications related to trailing spaces :/

SeanCurtis-TRI

Thanks for the contribution -- the windows side is definitely underserved and it's great that you're helping out your fellow windows users.

I can very much sympathize with your position on the white space. It'd be a real bear to back it out. In appreciation of your contribution, I'm going to slog through all of it so you don't have to change it. But you'd be doing me a huge favor in the future if your PRs don't include whitespace changes (unless, of course, it's nothing but). :)

Curiously, it was interesting to see the geographic patterns that appeared in the whitespace changes. Makes me wonder about the provenance of the clusters.

Reviewed 355 of 357 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

a discussion (no related file):
I didn't quite follow this:

Visual Studio expect dllexport in explicit template definition, while others compilers expect this kind of attribute in explicit template declaration.

Could you elaborate on what an "explicit template definition" is? What came to mind to me was explicit instantiation.

But then I'm not quite sure what "explicit template declaration" is.

CMakeLists.txt, line 34 at r1 (raw file):

Previously, SpaceIm wrote…

The only reason of 3.7 is the option CUSTOM_CONTENT_FROM_VARIABLE I use in generate_export_header in order to inject explicit template instantiation and definition definitions. There should be another way to achieve the same behaviour without this option.
For example, a less elegant solution could be to rename export.h, and create a file export.h with macros related to template instantiation which includes the file generated by generate_export_header. Thereby, we can rely of the current include behaviour of export.h in other files.

In this case, we're constrained enough that we'll have to cobble something together out of older technology. What do they say? True creativity arises under constraints? :)

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

FYI Some preprocessors need the # to be in the first column of the line, so indenting preprocessor definitions is usually done after the #.

Fair enough....in that case, I'd kill the subsequent indentation between "#" and "define".

jwnimmer-tri · 2020-04-09T22:43:00Z

Without implying any vote on whitespace process, FYI it's not difficult to fix. First, start from master and remove all trailing whitespace in a new commit, PR that, and merge to master. Then, rebase this PR atop that commit, always resolving conflicts by this PR winning (you can even just re-copy a backup of these files during the merge or rebase, don't even need to deal with diffs).

SpaceIm · 2020-04-09T22:48:51Z

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I didn't quite follow this:

Visual Studio expect dllexport in explicit template definition, while others compilers expect this kind of attribute in explicit template declaration.

Could you elaborate on what an "explicit template definition" is? What came to mind to me was explicit instantiation.

But then I'm not quite sure what "explicit template declaration" is.

ooops sorry, it's a typo: I meant explicit instantiation declaration and explicit instantiation definition.

SeanCurtis-TRI

I agree. However, it seemed to be that because every file with white space changes also has a non-whitespace change and then, when there are 350+ of them, separating the wheat from the chaff becomes non-trivial simply by the principle that small and simple times 350 is no longer small and simple. Hence my willingness to swallow it on this one.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

a discussion (no related file):

Previously, SpaceIm wrote…

ooops sorry, it's a typo: I meant explicit instantiation declaration and explicit instantiation definition.

No worries.

I hesitate to ask that but the symbols FCL_EXTERN_TEMPLATE_API and FCL_INSTANTIATION_DEF_API look very different for two concepts that are so closely related. Can we come up with names that better represents their relationship?

FCL_TEMPLATE_INSTANCE_DECLARE_API
FCL_TEMPLATE_INSTANCE_DEFINE_API

or

FCL_EXPLICT_INSTANCE_DECLARE_API
FCL_EXPLICT_INSTANCE_DEFINE_API

If we can come up with some names that appeal to everyone, then a quick search and replace in your IDE should be trivial. But let's not pull the trigger on that unless we come up with names we like more.

jwnimmer-tri · 2020-04-09T23:51:23Z

It's simple to git reset --hard OLDSHA to reset this branch after a rebase. It's O(1) effort not O(N).

SeanCurtis-TRI

That part is clear - but from where I'm sitting, all it does is take the PR and put it in "staged". But how do you take the staged changes and curate them into two commits? That's the part I consider to be time consuming and painful.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

jwnimmer-tri · 2020-04-10T00:13:26Z

Rough outline:
Part A:

git checkout master
git checkout -b whitespace-only
perl -pi -e 's# *$##g;' $(find . -name '*.hh' -o -name '*.cc')
# git add, git commit
# open a new PR with whitespace-only branch; review, CI, and merge it to master

Part B:

git checkout fix-shared-windows  # this current PR
cp -r src ../src.backup
cp -r include ../include.backup
git checkout master
git checkout -b fix-shared-windows-without-whitespace
rm -rf src include
mv ../src.backup src
mv ../include.backup include
# git add, git commit

Now fix-shared-windows-without-whitespace is identical to this PR, except rebased on whitespace changes.

sherm1 · 2020-04-10T03:01:02Z

That seems very straightforward! An alternate part B could be

# After the part A whitespace is merged into master:
git checkout master
git pull upstream master
git checkout fix-shared-windows  # this current PR
git rebase master  # using your favorite merge tool

Merging should be easy.

SeanCurtis-TRI

I feel foolish...I was limiting myself in considering untangling the commit it in terms of git operations. I hadn't considered duplicating the effect of the white space changes. :) And, as Sherm points out, once you've achieved the effect, a quick rebase and you're done.

I suspect the search and replace would have to be a bit more complex. I believe it's also making a new line character change in several files (a dozen or so). At least, that's my unconfirmed hypothesis.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

SeanCurtis-TRI

As I see this, the CMake formulation requiring CMake 3.7 is blocking this moving forward (the whitesapce, while a fun conversation, is resolved).

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

ahoarau · 2021-05-15T09:09:18Z

This PR works perfectly ! Thanks for the hard work.
I would love to see that integrated in upstream.
Indeed cmake is staying with an old version on xenial, but having the latest 3.20.2 is very easy since it only requires to put the archive's bin in the PATH.

That being said, I also understand the need of having only apt executables. How long do you plan to "support" cmake 3.5 ?

jamiesnape · 2021-05-17T13:03:02Z

Indeed cmake is staying with an old version on xenial, but having the latest 3.20.2 is very easy since it only requires to put the archive's bin in the PATH.

Also you can use the xenial APT packages from https://apt.kitware.com.

That being said, I also understand the need of having only apt executables. How long do you plan to "support" cmake 3.5 ?

I think we would be dropping support as soon as possible, but it has not been the highest priorities.

SeanCurtis-TRI

Do we have a straightforward solution to advancing this?

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

jamiesnape · 2021-05-19T20:08:49Z

Do we have a straightforward solution to advancing this?

At this stage, assuming that CI does not break: rebase, resolve conflicts, merge, and apologize to people using very old CMake (pre 3.7). If CI does break, I guess I could migrate it to GitHub Actions in an hour or two.

SteB0 · 2022-01-26T07:31:15Z

Hi,
thanks god that i found this issue!
After struggling with fcl inside my windows project i found @SpaceIm´s branch. Is someone working on a solution to fix the master version?

SpaceIm · 2022-01-26T10:15:01Z

Here is an attempt to solve merge conflict.

They are few things to fix which are not seen by merge conflict.

SpaceIm · 2022-01-26T12:14:20Z

I think the CMake min version issue is resolved since min version is 3.10 in master branch.

I've changed 3 more things:

hide all symbols by default => there is no reason to not hide private symbols.
rely on CMake abstraction to hide symbols.
allow to build tests when private symbols are hidden (I've also added enable_testing() at the root CMakeLists, it's convenient) => it works just fine.

@SeanCurtis-TRI I let you do your trick with rebase, split branch to remove whitespaces. But sorry, I won't solve merge conflicts again, it's too tedious and this PR is opened for 2 years.
By the way I still see trailing whitespaces in master, now I had to disable editorconfig. If your team doesn't follow your own .editorconfig (

fcl/.editorconfig

Line 41 in df2702c

trim_trailing_whitespace = true

), it would be nice to remove it to avoid to trick external contributors, specially if you are so strict about formatting in PR. Your clang-format doesn't seem to be honored as well in the code base. Fortunately, it was not activated at edit time in my IDE for fcl, otherwise it would have been a big mess to review the modifications.

sherm1 · 2022-01-27T17:55:20Z

I'd like to merge this heroic effort before code-rot sets in again! A few prerequisites

FCL CI seems broken. Does anyone know how to fix it?
We need at least one Windows user to verify that the most recent version solves the original problem. @SteB0 can you do that?

cc @SeanCurtis-TRI @jamiesnape @j-rivero @jslee02

sherm1 · 2022-01-27T21:19:21Z

Also cc @SeanCurtis-Drake

SteB0 · 2022-01-28T11:12:39Z

Hi, checked out latest version from @SpaceIm (265928f) and could build and use it without octomap and without Tests. After enabling "Build_Testing" in cmake-configuration i got linker errors e.g.
[46/81] Linking CXX executable test\test_fcl_collision.exe FAILED: test/test_fcl_collision.exe

undefined reference to 'fcl::detail::ConvertBVImpl<double, fcl::AABB<double>, fcl::OBB<double> >::run(fcl::AABB<double> const&, Eigen::Transform<double, 3, 1, 0> const&, fcl::OBB<double>&)'

SpaceIm · 2022-01-28T11:15:43Z

Which compiler? I think it's fine with msvc, I'll try with a GNU compiler.

SteB0 · 2022-01-28T11:22:50Z

mingw810_64
x86_64-w64-mingw32-g++.exe

SpaceIm · 2022-01-28T12:45:20Z

With apple-clang 13 on macOS, it works fine with FCL_HIDE_ALL_SYMBOLS=ON, FCL_STATIC_LIBRARY=OFF and BUILD_TESTING=ON. I use libccd v2.1 and eigen 3.4.0.
While it compiles and links fine, when I run tests, few of them fail in test_fcl_signed_distance, test_gjk_libccd-inl_epa, test_gjk_libccd-inl_gjk_doSimplex2 and test_gjk_libccd-inl_signed_distance. Moreover the last one is stuck at some point (in box_box test)

I'll try with MinGW (but I have gcc 11.2.0 with runtime 9.0.0 on my windows computer, mingw810_64 is pretty old).

ahoarau · 2022-01-28T13:19:44Z

Don't know if this is relevant for this MR, but I just found out that FCL can be made into a header-only library.
Can anyone explain the motivation behind declaring explicit templates in .cc ? Only speed when linking against it ?

SpaceIm · 2022-01-28T13:24:03Z

I would say speed up compilation of downstream projects, by avoiding common template instantiations in fcl.

SpaceIm · 2022-01-28T13:45:52Z

Ok so first thing to do to fix msvc shared with tests: turn test_fcl_utility to a static library.

And I also see link errors of test_fcl_collision with Visual Studio, but it's against a constructor of fcl::ContactPoint<double>.
I see, fcl::ContactPoint<double> doesn't declare nor define its destructor and copy/move constructors/assignement operators. They are not instantiated by Visual Studio during compilation, but at consume time explicit declaration tells to Visual Studio that they should be defined somewhere else so it doesn't try to instantiate them.

SpaceIm · 2022-01-28T15:09:10Z

Ok for Visual Studio, there were several missing declaration & definition of destructor, copy/move constructor/assignement in Contact & ContactPoint (though, it might be a bug in msvc). Moreover M_PI is not defined by default in cmath for Visual Studio, compilation of one test was failing.

SpaceIm · 2022-01-28T15:24:49Z

For MinGW, there are indeed some link issues in tests (test_collision_func_matrix for me) with ComputeBVImpl. I'll see what I can do.

SpaceIm · 2022-01-29T01:20:31Z

@SteB0 I think MinGW is fixed now with af00cc2 (may be tedious to review). The shared library builds and links, as well as the tests, even when all symbols are hidden.

Indeed order of declaration & definition of ComputeBVImpl templates was not correct. The proper order is:

template declaration
explicit instantiation declaration
template definition
explicit instantiation definition (in one and only one compilation unit)

SpaceIm · 2022-01-29T14:11:57Z

@SeanCurtis-TRI @jamiesnape @sherm1 I've fixed all the autoformatting mess, now only remain relevant modifications.

…act & ContactPoint

fix shared on windows

3b3a124

SeanCurtis-TRI requested changes Apr 7, 2020

View reviewed changes

SpaceIm commented Apr 7, 2020

View reviewed changes

SeanCurtis-TRI requested changes Apr 7, 2020

View reviewed changes

jamiesnape reviewed Apr 7, 2020

View reviewed changes

SpaceIm mentioned this pull request Apr 9, 2020

add fcl/0.6.1 conan-io/conan-center-index#1263

Merged

4 tasks

SeanCurtis-TRI requested changes Apr 9, 2020

View reviewed changes

SeanCurtis-TRI reviewed Apr 10, 2020

View reviewed changes

SeanCurtis-TRI reviewed Apr 22, 2020

View reviewed changes

SeanCurtis-TRI reviewed May 17, 2021

View reviewed changes

Merge branch 'master' into fix-shared-windows

54bf1ca

SpaceIm added 2 commits January 26, 2022 11:56

one more template to handle

1b5f9eb

simplify export macros

e61f38e

SpaceIm requested a review from SeanCurtis-TRI January 26, 2022 11:30

SpaceIm mentioned this pull request Jan 29, 2022

Avoid GLOB_RECURSE in CMakeLists #561

Open

SpaceIm added 10 commits January 29, 2022 17:03

rely on CMake properties to hide all symbols

ca4f5ae

allow to build test even if symbols are hidden

8b3fde5

hide all symbols by default

cd7fdfc

always build test_fcl_utility as a static library

0d4ca10

properly replace warning level /W3 by /W1 or /W4 for msvc

cfdddd9

add missing destructor, constructors & assignements operators in Cont…

f04a358

…act & ContactPoint

fix test_convex.cpp for msvc

c30d40f

allow to easily run tests on windows

09fecf8

fix MinGW

af00cc2

revert autoformatting

e9b313b

SpaceIm force-pushed the fix-shared-windows branch from 1f1a2cb to e9b313b Compare January 29, 2022 16:04

SpaceIm added 3 commits January 30, 2022 11:18

add missing explicit instantiation declaration

76a089b

define templates after explicit instantiation declaration

883b5da

better solution

fcecb85

fix shared on windows #461

Are you sure you want to change the base?

fix shared on windows #461

Conversation

SpaceIm commented Apr 7, 2020 • edited Loading

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SpaceIm left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

jamiesnape left a comment

Choose a reason for hiding this comment

jamiesnape left a comment

Choose a reason for hiding this comment

jamiesnape left a comment

Choose a reason for hiding this comment

SpaceIm commented Apr 9, 2020

SpaceIm commented Apr 9, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Apr 9, 2020

SpaceIm commented Apr 9, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Apr 9, 2020 • edited Loading

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Apr 10, 2020

sherm1 commented Apr 10, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

ahoarau commented May 15, 2021

jamiesnape commented May 17, 2021

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

jamiesnape commented May 19, 2021

SteB0 commented Jan 26, 2022

SpaceIm commented Jan 26, 2022 • edited Loading

SpaceIm commented Jan 26, 2022 • edited Loading

sherm1 commented Jan 27, 2022

sherm1 commented Jan 27, 2022

SteB0 commented Jan 28, 2022

SpaceIm commented Jan 28, 2022

SteB0 commented Jan 28, 2022

SpaceIm commented Jan 28, 2022 • edited Loading

ahoarau commented Jan 28, 2022

SpaceIm commented Jan 28, 2022

SpaceIm commented Jan 28, 2022 • edited Loading

SpaceIm commented Jan 28, 2022 • edited Loading

SpaceIm commented Jan 28, 2022 • edited Loading

SpaceIm commented Jan 29, 2022 • edited Loading

SpaceIm commented Jan 29, 2022

SpaceIm commented Apr 7, 2020 •

edited

Loading

jwnimmer-tri commented Apr 9, 2020 •

edited

Loading

SpaceIm commented Jan 26, 2022 •

edited

Loading

SpaceIm commented Jan 26, 2022 •

edited

Loading

SpaceIm commented Jan 28, 2022 •

edited

Loading

SpaceIm commented Jan 28, 2022 •

edited

Loading

SpaceIm commented Jan 28, 2022 •

edited

Loading

SpaceIm commented Jan 28, 2022 •

edited

Loading

SpaceIm commented Jan 29, 2022 •

edited

Loading